Skip to content

Add support for decimal types other than Foundation.Decimal #377

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add support for decimal types other than Foundation.Decimal #377

wants to merge 4 commits into from

Conversation

ratranqu
Copy link

Currently, the only numerical option for an exact representation of the Postgres NUMERIC type is to use Foundation.Decimal. However, Foundation.Decimal is not fully implemented, and in particular is missing some useful conformances such as LosslessStringConvertible or FloatingPoint. The current implementation makes it complex to use other Decimal implementations to map to the NUMERIC type.

This PR aims to provide a new protocol ExpressibleByFloatingPointString, to which Foundation.Decimal can trivially conform, and at the same time allow to easily make other Decimal types conform to (*), and be used as mapping to the Postgres NUMERIC type.

extension Decimal: ExpressibleByPostgresFloatingPointString {
    public init?(floatingPointString: String) {
        self.init(string: floatingPointString)
    }
}

The change aims to make no changes to the API. It has been tested with the Test suite from postgres-nio and that of postgres-kit as well as from use in a higher level Vapor app.

--
(*) for example BigDecimal from https://github.com/mgriebling/BigDecimal.git

import PostgresNIO
import BigDecimal

extension BigDecimal: ExpressibleByPostgresFloatingPointString {
    public init?(floatingPointString: String) {
        self.init(floatingPointString)
    } 
}

@gwynne gwynne requested review from fabianfett and gwynne July 27, 2023 20:16
@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2023 •
edited
Loading

Codecov Report

Attention: Patch coverage is 70.00000% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 61.72%. Comparing base (dc9caf8) to head (3eab477).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #377      +/-   ##
==========================================
- Coverage   61.82%   61.72%   -0.11%     
==========================================
  Files         125      126       +1     
  Lines       10019    10024       +5     
==========================================
- Hits         6194     6187       -7     
- Misses       3825     3837      +12
Files Coverage Δ
...PostgresNIO/New/Data/Decimal+PostgresCodable.swift 100.00% <100.00%> (+25.00%) ⬆️
...ources/PostgresNIO/Data/PostgresData+Decimal.swift 0.00% <0.00%> (ø)
...ources/PostgresNIO/Data/PostgresData+Numeric.swift 67.78% <0.00%> (-3.58%) ⬇️
...ata/ExpressibleByPostgresFloatingPointString.swift 76.00% <76.00%> (ø)

... and 2 files with indirect coverage changes

@gwynne
Copy link
Member

gwynne commented Jul 27, 2023 •
edited
Loading

It feels to me like this could be accomplished without a new protocol by making PostgresNumeric (which is already public) directly PostgresCodable; non-Foundation decimal types would then be able to more easily conform to PostgresCodable themselves using it. But I could easily be missing something. @fabianfett, any thoughts?

@ratranqu
Copy link
Author

ratranqu commented Jul 27, 2023 •
edited
Loading

It feels to me like this could be accomplished without a new protocol by making PostgresNumeric (which is already public) directly PostgresCodable; non-Foundation decimal types would then be able to more easily conform to PostgresCodable themselves using it. But I could easily be missing something. @fabianfett, any thoughts?

My gut feel is that as long as PostgresNumeric (an exact type) allows itself to be represented on the Swift side by Floats or Doubles (non exact) as well as Decimal/exact types, it will be difficult to allow for that since Float/Double have their own PostgresCodable specificities?

@ratranqu ratranqu force-pushed the add_support_for_other_decimal_types branch from bf7ea47 to 3eab477 Compare March 1, 2024 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabianfett fabianfett Awaiting requested review from fabianfett fabianfett is a code owner

@gwynne gwynne Awaiting requested review from gwynne gwynne is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ratranqu @codecov-commenter @gwynne